feat: add deprecate command with custom reason #921
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
2 Skipped Deployments
|
Lunaria Status Overview🌕 This pull request will trigger status changes. Learn moreBy default, every PR changing files present in the Lunaria configuration's You can change this by adding one of the keywords present in the Tracked Files
Warnings reference
|
📝 WalkthroughWalkthroughAdds package deprecation support across UI, CLI and server: a new Vue 3 component DeprecatePackageModal exposing Suggested reviewers
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
app/pages/package/[...package].vue (1)
1156-1169: Remove duplicate CSS classes.The button has redundant
inline-flex items-center(conflicting withflex) andw-fullappearing twice in the class string.🧹 Proposed fix
<button type="button" - class="flex items-center justify-center w-full px-3 py-1.5 bg-bg-subtle rounded text-sm font-mono text-red-400 hover:text-red-500 transition-colors inline-flex items-center gap-1.5 w-full" + class="flex items-center justify-center gap-1.5 w-full px-3 py-1.5 bg-bg-subtle rounded text-sm font-mono text-red-400 hover:text-red-500 transition-colors" `@click`="deprecateModal?.open()" >
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
app/pages/package/[...package].vue (1)
1245-1256: Clean up duplicate CSS classes.The button has redundant classes: both
flexandinline-flex, andw-fullappears twice.♻️ Suggested fix
<button type="button" - class="flex items-center justify-center w-full px-3 py-1.5 bg-bg-subtle rounded text-sm font-mono text-red-400 hover:text-red-500 transition-colors inline-flex items-center gap-1.5 w-full" + class="flex items-center justify-center gap-1.5 w-full px-3 py-1.5 bg-bg-subtle rounded text-sm font-mono text-red-400 hover:text-red-500 transition-colors" `@click`="deprecateModal?.open()" >
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
app/components/Package/DeprecatePackageModal.vue (2)
24-28: Use the importedtfunction instead of$tin the script block.The component destructures
tfromuseI18n()on line 12, but this computed uses$t. For consistency within the script setup, use the importedtfunction.♻️ Proposed fix
const modalTitle = computed(() => deprecateVersion.value - ? `${$t('package.deprecation.modal.title')} ${props.packageName}@${deprecateVersion.value}` - : `${$t('package.deprecation.modal.title')} ${props.packageName}`, + ? `${t('package.deprecation.modal.title')} ${props.packageName}@${deprecateVersion.value}` + : `${t('package.deprecation.modal.title')} ${props.packageName}`, )
121-127: Consider removing inline focus-visible utilities from buttons.Per project convention, focus-visible styling for buttons is applied globally via
main.csswithbutton:focus-visible { outline: 2px solid var(--accent); ... }. The inlinefocus-visible:outline-none focus-visible:ring-2 focus-visible:ring-fg/50utilities override this global rule.If this modal requires different focus styling, consider whether it should be consistent with the rest of the application or if an exception is warranted.
Based on learnings: "In the npmx.dev project, ensure that focus-visible styling for button and select elements is implemented globally in app/assets/main.css... Do not apply per-element inline utility classes."
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
app/components/Package/DeprecatePackageModal.vue (2)
69-69: Consider adding optional chaining for defensive access.If
state.value.operationsis ever undefined (e.g., during initialisation or after an error), this line would throw. Adding optional chaining aligns with the strict type-safety guideline.🛡️ Proposed fix
- const completedOp = state.value.operations.find(op => op.id === operation.id) + const completedOp = state.value.operations?.find(op => op.id === operation.id)
122-128: Remove inlinefocus-visibleutilities on buttons to rely on global styling.Both buttons in this component have custom
focus-visible:outline-none focus-visible:ring-2 focus-visible:ring-fg/50classes. The project uses a global CSS rule for button focus-visible styling inmain.css. Removing these inline utilities ensures consistency across the codebase.This applies to the button on lines 164-175 as well.
♻️ Proposed fix
<button type="button" - class="w-full px-4 py-2 font-mono text-sm text-fg-muted bg-bg-subtle border border-border rounded-md transition-colors duration-200 hover:text-fg hover:border-border-hover focus-visible:outline-none focus-visible:ring-2 focus-visible:ring-fg/50" + class="w-full px-4 py-2 font-mono text-sm text-fg-muted bg-bg-subtle border border-border rounded-md transition-colors duration-200 hover:text-fg hover:border-border-hover" `@click`="close" >And for the deprecate button:
<button type="button" :disabled="isDeprecating || !deprecateMessage.trim()" - class="w-full px-4 py-2 font-mono text-sm text-bg bg-fg rounded-md transition-colors duration-200 hover:bg-fg/90 disabled:opacity-50 disabled:cursor-not-allowed focus-visible:outline-none focus-visible:ring-2 focus-visible:ring-fg/50" + class="w-full px-4 py-2 font-mono text-sm text-bg bg-fg rounded-md transition-colors duration-200 hover:bg-fg/90 disabled:opacity-50 disabled:cursor-not-allowed" `@click`="handleDeprecate" >Based on learnings: "In the npmx.dev project, focus-visible styling for buttons and selects is applied globally via main.css... individual buttons or selects in Vue components should not rely on inline focus-visible utility classes."
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
…feat/resolve-40 * 'feat/resolve-40' of github.com:eryue0220/npmx.dev: Update app/components/Package/DeprecatePackageModal.vue
| describe('form state', () => { | ||
| it('deprecate button is disabled when message is empty', async () => { | ||
| const component = await mountSuspended(DeprecatePackageModal, { | ||
| props: { packageName: 'pkg' }, | ||
| ...mountOptions, | ||
| }) | ||
|
|
||
| const buttons = component.findAll('button[type="button"]') | ||
| const submitBtn = buttons.find(b => b.attributes('disabled') !== undefined) | ||
| expect(submitBtn).toBeDefined() | ||
| }) | ||
|
|
||
| it('deprecate button is enabled when message is filled and connected', async () => { | ||
| mockIsConnected.value = true | ||
| const component = await mountSuspended(DeprecatePackageModal, { | ||
| props: { packageName: 'pkg' }, | ||
| ...mountOptions, | ||
| }) | ||
|
|
||
| await component.find('#deprecate-message').setValue('Deprecated, use foo instead') | ||
| await component.vm.$nextTick() | ||
|
|
||
| const buttons = component.findAll('button[type="button"]') | ||
| const submitBtn = buttons.find(b => b.attributes('disabled') === undefined) | ||
| expect(submitBtn).toBeDefined() | ||
| }) | ||
|
|
||
| it('handleDeprecate does nothing when not connected', async () => { | ||
| mockIsConnected.value = false | ||
| const component = await mountSuspended(DeprecatePackageModal, { | ||
| props: { packageName: 'pkg' }, | ||
| ...mountOptions, | ||
| }) | ||
|
|
||
| await component.find('#deprecate-message').setValue('message') | ||
| await component.vm.$nextTick() | ||
| await getVM(component).handleDeprecate?.() | ||
|
|
||
| expect(mockAddOperation).not.toHaveBeenCalled() | ||
| }) | ||
|
|
||
| it('handleDeprecate does nothing when message is empty', async () => { | ||
| const component = await mountSuspended(DeprecatePackageModal, { | ||
| props: { packageName: 'pkg' }, | ||
| ...mountOptions, | ||
| }) | ||
|
|
||
| await getVM(component).handleDeprecate?.() | ||
|
|
||
| expect(mockAddOperation).not.toHaveBeenCalled() | ||
| }) | ||
| }) |
There was a problem hiding this comment.
Target the deprecate action button explicitly in enable/disable assertions.
The current selector can match the cancel button, so these tests can pass even if the deprecate button’s state is wrong. Prefer a dedicated selector (e.g. a data-testid on the deprecate button) and assert directly on it.
Proposed fix (use a dedicated selector)
- const buttons = component.findAll('button[type="button"]')
- const submitBtn = buttons.find(b => b.attributes('disabled') !== undefined)
- expect(submitBtn).toBeDefined()
+ const submitBtn = component.get('[data-testid="deprecate-submit"]')
+ expect(submitBtn.attributes('disabled')).toBeDefined()- const buttons = component.findAll('button[type="button"]')
- const submitBtn = buttons.find(b => b.attributes('disabled') === undefined)
- expect(submitBtn).toBeDefined()
+ const submitBtn = component.get('[data-testid="deprecate-submit"]')
+ expect(submitBtn.attributes('disabled')).toBeUndefined()1 participant
resolve #40